feat: restrict directories for included files in the cluster templates#2608
Open
Unix4ever wants to merge 1 commit intosiderolabs:mainfrom
Open
feat: restrict directories for included files in the cluster templates#2608Unix4ever wants to merge 1 commit intosiderolabs:mainfrom
Unix4ever wants to merge 1 commit intosiderolabs:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Unix4ever
added
the
integration/e2e-templates
There was a problem hiding this comment.
Pull request overview
Adds allow-list based path validation for files referenced by cluster templates to mitigate malicious templates including arbitrary local files (e.g., /etc/passwd).
Changes:
- Introduces
AllowedFilePathsvalidation options and threads them through template/model validation. - Adds
ValidateFilePathhelper + unit tests, and extends template tests for allowed-path behavior. - Adds
--allowed-pathsomnictl flag and wires it into template sync/diff flows.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| client/pkg/testdata/parent-patch.yaml | Adds test fixture patch file located outside template testdata dir. |
| client/pkg/template/template.go | Adds ValidateOptions and passes allow-list to model validation. |
| client/pkg/template/template_test.go | Updates validation calls and adds allow-list validation coverage. |
| client/pkg/template/operations/validate.go | Updates validation call site (currently passes empty options). |
| client/pkg/template/operations/sync.go | Adds AllowedFilePaths to SyncOptions and passes through to validation. |
| client/pkg/template/operations/status.go | Updates validation call site (currently passes empty options). |
| client/pkg/template/operations/render.go | Updates validation call site (currently passes empty options). |
| client/pkg/template/operations/export.go | Updates model list validation signature to accept options. |
| client/pkg/template/operations/diff.go | Adds allowedFilePaths parameter and passes through to validation. |
| client/pkg/template/operations/delete.go | Updates validation call site (currently passes empty options). |
| client/pkg/template/internal/models/workers.go | Threads validation options down to patch validation. |
| client/pkg/template/internal/models/validate_path.go | Implements allow-list directory check for referenced files. |
| client/pkg/template/internal/models/validate_path_test.go | Adds unit tests for file path allow-list behavior. |
| client/pkg/template/internal/models/patch.go | Adds allow-list check before reading patch files during validation. |
| client/pkg/template/internal/models/models.go | Adds ValidateOptions and updates Model interface signature. |
| client/pkg/template/internal/models/machine.go | Threads validation options down to patch validation. |
| client/pkg/template/internal/models/list.go | Threads validation options to all models during list validation. |
| client/pkg/template/internal/models/kubernetes_manifest.go | Adds allow-list check for manifest file paths during validation. |
| client/pkg/template/internal/models/controlplane.go | Threads validation options down to patch validation. |
| client/pkg/template/internal/models/cluster.go | Threads validation options down to patches/manifests validation. |
| client/pkg/omnictl/cluster/template/template.go | Adds --allowed-paths flag and resolves relative paths against template directory. |
| client/pkg/omnictl/cluster/template/sync.go | Sets AllowedFilePaths per processed template file before syncing. |
| client/pkg/omnictl/cluster/template/diff.go | Passes resolved allowed paths into diff operation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Unix4ever
force-pushed
the
restrict-included-files-scope-cluster-templates
branch
from
b561e82 to
53a6061
Compare
Unix4ever
force-pushed
the
restrict-included-files-scope-cluster-templates
branch
from
53a6061 to
ea9585c
Compare
smira
reviewed
Apr 6, 2026
| // | ||
| // Symlinks in the file path are resolved to prevent bypassing the allowed path restriction | ||
| // (e.g., a symlink ./patches/passwd -> /etc/passwd would be caught). | ||
| func ValidateFilePath(filePath string, allowedPaths []string) error { |
Member
There was a problem hiding this comment.
I think if we really want to be strict about this check, we should do it not on validation, but the moment we try to open the file.
When we do validation, e.g. a symlink might be fine, but potentially something might replace the symlink target after the validation.
Also, from security perspective, I think it's better to use https://pkg.go.dev/os#Root as it already has lots of logic to prevent reads outside of the root.
There might be an easier model if we allow a single root (allowedPaths is a single entry), and open any file we need from within that root throughout the whole module.
Unix4ever
force-pushed
the
restrict-included-files-scope-cluster-templates
branch
from
ea9585c to
3d52e1a
Compare
utkuozdemir
reviewed
Apr 9, 2026
|
|
||
| modelList := buildModelList(clusterModel, controlPlaneMachineSetModel, workerMachineSetModels, machineModels) | ||
| if err = modelList.Validate(); err != nil { | ||
| if err = modelList.Validate(models.ValidateOptions{}); err != nil { |
Member
There was a problem hiding this comment.
I like that we simply use an options struct, no functional args pattern etc.
By default only allow to include files from the same directory where the template file lives. This is to prevent malicious cluster templates that include something like `/etc/passwd`. Fixes: siderolabs#2590 Signed-off-by: Artem Chernyshev <artem.chernyshev@talos-systems.com>
Unix4ever
force-pushed
the
restrict-included-files-scope-cluster-templates
branch
from
3d52e1a to
22df2c0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
By default only allow to include files from the same directory where the template file lives.
This is to prevent malicious cluster templates that include something like
/etc/passwd.Fixes: #2590